Skip to content

chore(layers): bundle assets from source when testing #1710

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Sep 25, 2023

Conversation

dreamorosi
Copy link
Contributor

@dreamorosi dreamorosi commented Sep 25, 2023

Description of your changes

This PR introduces some changes to the way the Lambda Layers are built when running integration tests. The changes should be backward compatible with the current release process, which should work as before for the most part.

Below a matrix of the changes and the areas they are supposed to impact:

Integration Release
Bundling Script
Lambda Function
Build Steps

Bundling Script

Before this PR we had a bash script present in the .github/scripts/ folder that had to be run before deploying the layers or running tests.

This was fine in the CI environment since it was only an additional step. When working locally however we didn't have any automated way of running it and contributors/maintainers had to remember to run it before being able to run the integration tests for the layers workspace.

This PR removes this script in favor of using CDK local bundling. The Code.fromAsset() method that we use to reference the layer content accepts two arguments: the location of the asset and a bundling configuration object.

Even though this is not really well documented, it's possible to force the bundling to happen locally and without Docker. This is exactly what the NodejsFunction L3 construct does under the hood and also what described here in passing.

The command looks something like this, but with more execSync commands:

code: Code.fromAsset(resolve(__dirname), {
  bundling: {
    // This is here only because is required by CDK, however it is not used since the bundling is done locally
    image: Runtime.NODEJS_18_X.bundlingImage,
    // We need to run a command to generate a random UUID to force the bundling to run every time
    command: [`echo "${randomUUID()}"`],
    local: {
      tryBundle(outputDir: string) {
        execSync('...'); // bash command to execute as part of the bundling
      }
   }
}  

With this method we don't have to maintain two scripts and the build process is always run automatically as part of the synth process 🚀 .

Lambda Function

Before this release we were performing the following checks:

  • The version in the package.json file of one utility (Logger) matches the expected version
  • Import & initialize the utilities
  • Test Logger by emitting a debug log
  • Test Metrics by adding a metric (ColdStart)
  • Test Tracer by interacting with a segment

This PR changes the first and last actions to:

  • Check that the version in the package.json files of each utility present in the layer matches the expected one
  • Test that the tracer actually works by logging the segment's content, so that it can be asserted in the unit tests

The first one is mainly for an excess of caution, however the second was needed because most of the methods in the Tracer are treated as non-critical and as such they are not meant to throw if they don't succeed. By logging the content of a segment we can test that the actions we perform actually happened.

Build Steps

As mentioned in the linked issue, before this PR we didn't have any means to test the Powertools utilities in a Layer without making a release. This PR introduces a new CDK context variable called BuildFromSource that allows to tell the CDK construct to build the packages from sources before deploying the Lambda Layer (defaults to false).

This behavior is set to true in the integration tests but defaults to false in the release pipeline. When BuildFromSource is true, the build process runs npm run build -w packages/<utility> and npm pack -w packages/<utility>, then uses the resulting tarballs to install the utility in the layer folder.

This not only allows us to test the code we are working on in the integration tests, but also serves as an early canary for the build/pack process itself.

Related issues, RFCs

Issue number: closes #1709

Checklist

  • My changes meet the tenets criteria
  • I have performed a self-review of my own code
  • I have commented my code where necessary, particularly in areas that should be flagged with a TODO, or hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my change is effective and works
  • The PR title follows the conventional commit semantics

Breaking change checklist

Is it a breaking change?: NO

  • I have documented the migration process
  • I have added, implemented necessary warnings (if it can live side by side)

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Disclaimer: We value your time and bandwidth. As such, any pull requests created on non-triaged issues might not be successful.

@dreamorosi dreamorosi requested a review from a team September 25, 2023 17:41
@dreamorosi dreamorosi self-assigned this Sep 25, 2023
@boring-cyborg boring-cyborg bot added automation This item relates to automation layers Items related to the Lambda Layers pipeline dependencies Changes that touch dependencies, e.g. Dependabot, etc. internal PRs that introduce changes in governance, tech debt and chores (linting setup, baseline, etc.) tests PRs that add or change tests labels Sep 25, 2023
@pull-request-size pull-request-size bot added the size/L PRs between 100-499 LOC label Sep 25, 2023
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@am29d
Copy link
Contributor

am29d commented Sep 25, 2023

Ah, I have discovered exactly the same issue when working on #1703 because we can't test layer without having a release. Now we can combine both solutions.

Copy link
Contributor

@am29d am29d left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Outstanding refactoring! I was working on improving the shell script, but you took it a step further, well done!

I was tempted to add the packages and adjust the function code here, but I will address it in a separate PR.

@am29d am29d merged commit ccc0398 into main Sep 25, 2023
@am29d am29d deleted the chore/layers/bundleAsset branch September 25, 2023 19:17
@dreamorosi
Copy link
Contributor Author

Thanks, glad you agree!

I thought about that too but the issue was assigned to you and saw you were already pretty far ahead with the tests so you should take credit for that work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automation This item relates to automation dependencies Changes that touch dependencies, e.g. Dependabot, etc. internal PRs that introduce changes in governance, tech debt and chores (linting setup, baseline, etc.) layers Items related to the Lambda Layers pipeline size/L PRs between 100-499 LOC tests PRs that add or change tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Maintenance: build Lambda Layers from source during tests
2 participants